Skip to content

Fixed issue #15192 #15241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 20, 2025
Merged

Fixed issue #15192 #15241

merged 1 commit into from
Jul 20, 2025

Conversation

lkshayb
Copy link
Contributor

@lkshayb lkshayb commented Jul 10, 2025

Fixes #15192 by adding checks for no_std while giving out Box recommendation

changelog: [`large_enum_variant`]: Dont suggest `Box` in `no_std` mode

@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2025

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 10, 2025
Copy link

github-actions bot commented Jul 10, 2025

Lintcheck changes for b738d96

Lint Added Removed Changed
clippy::large_enum_variant 0 0 13

This comment will be updated if you push new changes

@samueltardieu
Copy link
Member

The changelog line in the PR description should contain the lint name in square brackets and quotes, such as in

changelog: [`large_enum_variant`]: do not suggest `Box` in `no_std` mode

(note also the quotes around Box and no_std)

This way it will be easily transformed into a link to the lint description when integrated in CHANGELOG.md when we issue the release notes.

@lkshayb
Copy link
Contributor Author

lkshayb commented Jul 10, 2025

@samueltardieu Thanks ! I've updated the changelog line as suggested .

Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I personally feel like the help message for what we're expecting the user to do to resolve the lint (introduce indirection for example by boxing) was still helpful as a hint.

Even in #![no_std] crates, one can still use extern crate alloc and use alloc::boxed::Box<_> that way (and I'd expect in practice this suggestion is very often still applicable), and even if the user doesn't have the alloc crate, I'd expect there would still be some other moral equivalent to introduce indirection

What do you think about instead of omitting this entirely in #![no_std] crates, actually "downgrade" the suggestion to just the help message, so it still hints to the user what they can do about it. We could also slightly change the message in case the user really can't use Box<_>. I'm thinking of something like:

help: consider boxing the large fields or introducing indirection in some other way to reduce the total size of the enum

@@ -0,0 +1,16 @@
#![no_std]
#![no_main]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are built as library crates, so you should be able to largely simplify this. The #![no_main] and panic handling code shouldn't be needed.

#![no_std]
#![warn(clippy::large_enum_variant)]

enum Myenum {
    //~^ ERROR: large size difference between variants
    Small(u8),
    Large([u8; 1024]),
}

@lkshayb
Copy link
Contributor Author

lkshayb commented Jul 11, 2025

@y21 Thanks for the feedback . You are correct that a downgraded help message should be used . I will update my pull request to follow the same and also I'll edit out the panic handler and #[no_main].

@lkshayb
Copy link
Contributor Author

lkshayb commented Jul 11, 2025

@y21 I have implemented the suggestions you made . Please review it now.

@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 12, 2025
@rustbot

This comment has been minimized.

@rustbot rustbot removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) has-merge-commits PR has merge commits, merge with caution. labels Jul 13, 2025
@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 13, 2025
@rustbot

This comment has been minimized.

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 14, 2025
@rustbot

This comment has been minimized.

@lkshayb lkshayb force-pushed the master branch 2 times, most recently from ca8e2a3 to 2614a31 Compare July 14, 2025 09:08
@lkshayb
Copy link
Contributor Author

lkshayb commented Jul 16, 2025

@rustbot r? @y21

@rustbot
Copy link
Collaborator

rustbot commented Jul 16, 2025

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also squash the commits into one?

@@ -149,7 +150,9 @@ impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant {
return;
}
}
diag.span_help(def.variants[variants_size[0].ind].span, help_text);
if !is_no_std_crate(cx) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this if here on purpose? My suggestion was to always at least show the help message, no matter what, as a guidance in case the user has some other way to add indirection, even without access to alloc.

As is implemented right now, it will still suppress the help message entirely in #![no_std] (as can be seen in the test)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looking at the git history on this branch, it looks like this was correct in lkshayb@0ac806d , but was then later reverted again in lkshayb@3e6d575

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx @y21 for pointing this out . I've updated the pr now . and also squashed all the commits into one.

@lkshayb
Copy link
Contributor Author

lkshayb commented Jul 18, 2025

@y21 I've implemented the changes and squashed all the commits into one , please review now .

Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@y21 y21 added this pull request to the merge queue Jul 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 19, 2025
@y21
Copy link
Member

y21 commented Jul 19, 2025

Ah, looks like there's a 32-bit stderr of the test that wasn't blessed. They're a bit annoying to bless on 64 bit, but in this case since it's just the help message that changed everywhere, you could also just do a manual replace in tests/ui/large_enum_variant.32bit.stderr.

Also, while you're fixing that, minor nit: could you also reword the commit to something more descriptive? Thanks!

@lkshayb lkshayb force-pushed the master branch 2 times, most recently from 0488d7f to d5bc095 Compare July 20, 2025 08:11
- Updated the `help:` text to include a more general suggestion
- Ensures compatibility with `#[no_std]` environments where boxing is
  still possible via `alloc`.
@lkshayb
Copy link
Contributor Author

lkshayb commented Jul 20, 2025

@y21 I've blessed the 32 bit stederr manually and also changed the commit message .

@y21 y21 added this pull request to the merge queue Jul 20, 2025
Merged via the queue into rust-lang:master with commit c8e333c Jul 20, 2025
11 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

large_enum_variant propose a wrong solution for no_std
4 participants